Skip to content

Directly call in-library functions to build packages #9871

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Directly call in-library functions to build packages #9871

wants to merge 3 commits into from

Conversation

sheaf
Copy link
Collaborator

@sheaf sheaf commented Apr 8, 2024 •
edited
Loading

Template Β: This PR does not modify cabal behaviour (documentation, tests, refactoring, etc.)

This PR modifies cabal-install to directly go through the Cabal library when building packages, instead of the Setup interface (except of course for build-type: Custom packages).

Overview of changes:

  • Addition of LibraryMethod to SetupMethod(*), in which we directly invoke the Cabal configure, build, ... functions.
    • This required a bit of GADT trickery to accomodate the fact that configure returns a LocalBuildInfo which must then be passed to subsequent phases, while with the old Setup interface everything returns IO () and communication is done through the filesystem (e.g. the local build info file).
  • New library hooks-exe, provisioning:
    • CallHooksExe: the API for communicating with an external hooks executable (cabal-install depends on this)
    • HooksExe: the necessary functions for creating a hooks executable (cabal-install adds a dependency on this when compiling a package with build-type: Hooks).
    • This library depends on the new CommunicationHandle functionality from process PR #308.

(*) NB: SetupMethod/SetupWrapper is now a bit of a misnomer, because the whole aim of this PR is to no longer go through the Setup interface (and its old UserHooks incarnations such as defaultMainWithHooks ). New naming conventions welcome!
The main change is in Distribution.Client.SetupWrapper: we add a new SetupMethod (probably a misnomer at this point because we no longer go through Setup; new names for this datatype are welcome) in which we directly invoke the Cabal configure, build etc functions rather than going through Setup (or the equivalent defaultMainWithUserHooks interface).

TODO:

@sheaf sheaf marked this pull request as draft April 8, 2024 10:45
@andreabedini
Copy link
Collaborator

I tried compiling this with

source-repository-package
  type: git
  location: https://github.com/sheaf/process
  tag: 61b1de95e72acda5d773c972c7484b1a792ca9e5
  post-checkout-command: autoreconf -i

but it fails because System.Process.CommunicationHandle in process does not export useCommunicationHandle. Am I missing something?

@sheaf sheaf force-pushed the wip/cabal-install-hooks branch 2 times, most recently from 253e4d5 to d4a6532 Compare April 11, 2024 08:47
@sheaf
Copy link
Collaborator Author

sheaf commented Apr 11, 2024

I tried compiling this with

source-repository-package
  type: git
  location: https://github.com/sheaf/process
  tag: 61b1de95e72acda5d773c972c7484b1a792ca9e5
  post-checkout-command: autoreconf -i

but it fails because System.Process.CommunicationHandle in process does not export useCommunicationHandle. Am I missing something?

I've updated the PR now, but please note that this PR is still work in progress and there are quite a few issues that I have yet to resolve.

I didn't know about post-checkout-command, that's neat.

@sheaf sheaf force-pushed the wip/cabal-install-hooks branch 8 times, most recently from 17278dc to c209ca5 Compare April 17, 2024 16:42
@sheaf sheaf force-pushed the wip/cabal-install-hooks branch 5 times, most recently from 7392807 to bbc2418 Compare April 29, 2024 15:14
@sheaf sheaf force-pushed the wip/cabal-install-hooks branch 5 times, most recently from e34ac10 to 0085999 Compare May 3, 2024 12:01
@sheaf sheaf changed the title Draft: SetupHooks integration for cabal-install Draft: Directly call in-library functions to build packages May 3, 2024
@sheaf sheaf force-pushed the wip/cabal-install-hooks branch 3 times, most recently from 2469702 to c3554cd Compare May 6, 2024 13:40
@sheaf
Copy link
Collaborator Author

sheaf commented May 6, 2024

I have successfully built all of the clc-stackage repository with this patched version of cabal (I included an additional patch to ignore logging handles, to avoid using self-exec instead of in-library build method).

@sheaf sheaf force-pushed the wip/cabal-install-hooks branch 2 times, most recently from ee74d63 to 243f1c8 Compare May 7, 2024 10:51
@sheaf sheaf force-pushed the wip/cabal-install-hooks branch 2 times, most recently from 840f8d4 to d822b63 Compare June 17, 2025 10:17
@mmhat
Copy link

mmhat commented Jun 17, 2025

The current state of this work is that it is blocked on a deeper refactoring of the Cabal configure logic which would split up the configure function to allow us to directly call the parts we want to do on a per-package level without re-doing things such as configuring the compiler.

Are there any related issues to that effort? I am trying to figure out at when (broadly, of course) this PR here may land.

@sheaf sheaf mentioned this pull request Jun 18, 2025
Merged
1 task
@sheaf sheaf force-pushed the wip/cabal-install-hooks branch from d822b63 to 6016670 Compare June 18, 2025 11:41
@sheaf
Copy link
Collaborator Author

sheaf commented Jun 18, 2025 •
edited
Loading

Are there any related issues to that effort? I am trying to figure out at when (broadly, of course) this PR here may land.

I implemented the refactoring I was talking about in that comment in this PR, by refactoring the configure function from the Cabal library to expose the new configureFinal function, which does some necessary configuration (e.g. backpack instantiation logic) without re-doing configuration of things we already know such as rediscovering which compiler we are using, re-computing the program database we are using, etc.

I've been thoroughly testing this PR by diffing the behaviour between HEAD and this patch when building all of stackage. I've eliminated all the inconsistencies I could find, so I think this work is about ready for another round of review.

@sheaf sheaf force-pushed the wip/cabal-install-hooks branch from 6016670 to 8f65ab9 Compare June 18, 2025 11:52
@sheaf sheaf marked this pull request as ready for review June 18, 2025 13:50
geekosaur
geekosaur approved these changes Jun 18, 2025
View reviewed changes
Copy link
Collaborator

@geekosaur geekosaur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not real confident that I understood everything (or that I covered everything given the size of the diffs).

@Mikolaj
Copy link
Member

Mikolaj commented Jun 19, 2025

@sheaf: would it be possible to merge the other PRs this one depend on first, to make reviewing easier? In particular, it's hard to see if the test changes indicate there are behaviour changes or not. Thanks!

@sheaf sheaf marked this pull request as draft July 14, 2025 09:53
@sheaf sheaf force-pushed the wip/cabal-install-hooks branch 7 times, most recently from bd1670a to f1fe208 Compare July 14, 2025 13:27
sheaf and others added 3 commits July 14, 2025 16:49
@sheaf
cabal-install: call Cabal in-library
7c1a03d 
This commit modifies the SetupWrapper mechanism, adding a new way of
building a package: directly calling Cabal library functions (e.g.
'build', 'configure' etc).

This currently requires a bit of GADT trickery to accomodate the fact
that configure returns a LocalBuildInfo which must then be passed to
subsequent phases, while with the old Setup interface everything returns
IO () and communication is done through the filesystem
(the local build info file).

To handle 'build-type: Hooks', this commit introduces the hooks-exe
package, which contains:

  - the hooks-exe library, used to compile a set of SetupHooks into an
    external executable,
  - the hooks-cli library, which is used by cabal-install to communicate
    with an external hooks executable.

This package depends on the new `CommunicationHandle` functionality from
haskell/process#308.
@alt-romes @sheaf
hooks: Implicitly depend on hooks-exe
9cea27f 
The `hooks-exe` package enables `SetupHooks` values to be converted into
a `Setup.hs` executable which can be executed independently of Cabal.
The `Setup.hs` executable wrapping `SetupHooks` is quite important to
preserve the interface used by other tools when packages migrate to
`Hooks` from `Custom`.

Even though `hooks-exe` is an internal dependency required by the `Setup.hs`
wrapper around `SetupHooks`, it is a dependency nonetheless.

Given the internal nature of `hooks-exe`, we don't want to impose on our
users the obligation to add a dependency on `hooks-exe` in their
setup-depends field. Instead, we want `hooks-exe` to be implicitly added
to the setup dependencies. This commit does that exactly.
@sheaf
Update bootstrap plans for hooks-exe
76cc30c 
This commit updates the bootstrap plans to account for the new
local hooks-exe package, which cabal-install depends on.
@sheaf sheaf force-pushed the wip/cabal-install-hooks branch from f1fe208 to 76cc30c Compare July 14, 2025 14:49
@ffaf1
Copy link
Collaborator

ffaf1 commented Aug 28, 2025

Since this is a draft now, I have removed the needs-review label.

@mpickering mpickering mentioned this pull request Sep 15, 2025
Open
@ulysses4ever
Copy link
Collaborator

@sheaf do you think you'll get back to this PR and rebase / address the comments above? It feels like a great refactoring that we really want to merge! But there's some boring work left...

@sheaf sheaf mentioned this pull request Oct 6, 2025
Draft
6 tasks
@sheaf
Copy link
Collaborator Author

sheaf commented Oct 6, 2025

@sheaf do you think you'll get back to this PR and rebase / address the comments above? It feels like a great refactoring that we really want to merge! But there's some boring work left...

I agree, it would be good to make progress here. As far as I'm concerned, the main reason for holding off was that no one would benefit because of issues to do with logging handles (we wouldn't use the "in-library" code path whenever a logging handle is being used, which is usually the case in practice). That issue is resolved by #11077, which is functionally ready but needs finishing up.

@ulysses4ever
Copy link
Collaborator

Ah, I didn't realise the connection to #11077, thanks for clarifying that! #11077 is a pretty heavy (even if straightforward) refactoring. Let us know when you think it's ready for a review (you should be able to set the needs review label, or just leave a comment...).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreabedini andreabedini andreabedini left review comments

@alt-romes alt-romes alt-romes left review comments

@geekosaur geekosaur geekosaur approved these changes

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@sheaf @andreabedini @mmhat @Mikolaj @ffaf1 @ulysses4ever @geekosaur @alt-romes